Skip to content

Add ssqosid extension and csr yaml #653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

syedowaisalishah
Copy link
Contributor

This PR adds:

  • Extension YAML file for Ssqosid
  • CSR YAML file for Ssqosid: srmcfg

Closes #567

syedowaisalishah and others added 3 commits May 2, 2025 08:00
Signed-off-by: Syed Owais Ali Shah <113125582+syedowaisalishah@users.noreply.github.com>
Signed-off-by: Syed Owais Ali Shah <113125582+syedowaisalishah@users.noreply.github.com>
@syedowaisalishah syedowaisalishah requested a review from dhower-qc May 9, 2025 21:07
@syedowaisalishah
Copy link
Contributor Author

@ThinkOpenly, I tested this using all files from PR #592. The first issue I noticed was a typo in mstateen0.yaml
definedBy:
squosid should be corrected to Ssqosid.

Also, I'm getting a type error on RV32:

CSR[mstateen0].SRMCFG is not defined in RV32

But SRMCFG is defined in mstateen0.yaml like this:

fields:
  SRMCFG:
    location: 55
    definedBy: Ssqosid
    base: 64
    ...

Should I add something to explicitly include it for RV32? Or is there a better way to handle this?

@ThinkOpenly
Copy link
Collaborator

@ThinkOpenly, I tested this using all files from PR #592. The first issue I noticed was a typo in mstateen0.yaml definedBy: squosid should be corrected to Ssqosid.

Are you on top of the latest commits for PR #592? The only references to "Ssqosid" are commented out.

Also, I'm getting a type error on RV32:

CSR[mstateen0].SRMCFG is not defined in RV32

But SRMCFG is defined in mstateen0.yaml like this:

fields:
  SRMCFG:
    location: 55
    definedBy: Ssqosid
    base: 64
    ...

Should I add something to explicitly include it for RV32? Or is there a better way to handle this?

It could be interesting to read the most recent discussion in #592. I think what we're going to need to define the "upper half" fields of those "state enable" CSRs in the "upper half" versions of those registers. Currently, they're defined as aliases, but that's not working at the moment.

Regardless, for this situation, you'll need two cases, something like:

if SXLEN > 32
  # Use CSR[mstateen0].SRMCFG
else
  # Use CSR[mstateen0h].SRMCFG

...but I confess I'm not sure that'l work because the code it still referencing the field from mstateen0, even if it's really not if SXLEN == 32. It depends on how smart the IDL compiler is, I think. It's worth a try, but @dhower-qc may need to chime in here.

@syedowaisalishah
Copy link
Contributor Author

Are you on top of the latest commits for PR #592?

Yes, I'm on the latest commit for PR #592.

@ThinkOpenly
Copy link
Collaborator

Yes, I'm on the latest commit for PR #592.

This is what I see in PR #592's latest version of mstateen0.yaml:

  SRMCFG:
    long_name: srmcfg access control
    location: 55
    #definedBy: Ssquosid
    base: 64
    description: |
      The SRMCFG bit in `mstateen0` controls access to the `srmcfg`` CSR introduced by the Ssqosid Chapter 18
      extension.
    type: RW
    reset_value: 0

Is the typo in there? I'm not seeing it.

@syedowaisalishah
Copy link
Contributor Author

Yes, there is a typo in the commented-out line:

#definedBy: Ssquosid

It should be:

#definedBy: Ssqosid

Just a small transposition of letters. Everything else looks good.

@ThinkOpenly
Copy link
Collaborator

It should be:

#definedBy: Ssqosid

Ah! I see it now. Amazing how that hides to my eyes. Could you add a review comment in #592 for that?

@neverlandiz
Copy link
Contributor

It should be fixed now, sorry about that!

@ThinkOpenly
Copy link
Collaborator

I'm getting a type error on RV32:

CSR[mstateen0].SRMCFG is not defined in RV32

It could be interesting to read the most recent discussion in #592. I think what we're going to need to define the "upper half" fields of those "state enable" CSRs in the "upper half" versions of those registers. Currently, they're defined as aliases, but that's not working at the moment.

Regardless, for this situation, you'll need two cases, something like:

So, ignore all that. There's much more discussion in #592 and the other more distant issues referenced there.

The current understanding is that #592 will change to make that field defined for RV32, and the code emitting the type error will become correct. So, stand by.

@ThinkOpenly
Copy link
Collaborator

I believe #592 has been updated with the changes you needed here. Are you able to rebase on that now?

@ThinkOpenly
Copy link
Collaborator

I believe #592 has been updated with the changes you needed here. Are you able to rebase on that now?

#592 is in the merge queue now, should be merged soon.

@syedowaisalishah
Copy link
Contributor Author

@ThinkOpenly,Yes, I’ll check today after pulling the latest changes from the #592 merge and running the tests. I’ll update accordingly.

@syedowaisalishah
Copy link
Contributor Author

@ThinkOpenly I got this error in sw_write():

On line 115:
rcid_mask = (1 << RCID_WIDTH) - 1;
return csr_value.RCID & rcid_mask;

Error: no symbol named 'rcid_mask' on line 115

Context code:

sw_write(csr_value): |
  if (implemented?(ExtensionName::Smstateen)) {
    if (mode() < PrivilegeMode::M && CSR[mstateen0].SRMCFG == 0) {
      raise(ExceptionCode::IllegalInstruction, mode(), $encoding);
    }
    if (virtual_mode?() && CSR[mstateen0].SRMCFG == 1) {
      raise(ExceptionCode::VirtualInstruction, mode(), $encoding);
    }
  } else {
    if (virtual_mode?()) {
      raise(ExceptionCode::VirtualInstruction, mode(), $encoding);
    }
  }
  rcid_mask = (1 << RCID_WIDTH) - 1;
  return csr_value.RCID & rcid_mask;

How should I define rcid_mask properly in IDL?
Could you please guide me on how to correctly reference global parameters like RCID_WIDTH.

@ThinkOpenly
Copy link
Collaborator

On line 115:
rcid_mask = (1 << RCID_WIDTH) - 1;
return csr_value.RCID & rcid_mask;

Error: no symbol named 'rcid_mask' on line 115

There are many examples of declaring variables in other CSR files.

In this particular case, maybe you could just avoid using a variable:

return csr_value.RCID & ((1 `<< RCID_WIDTH) - 1);

Could you please guide me on how to correctly reference global parameters like RCID_WIDTH.

Refer to other implementations. Search for "WIDTH" in arch/ext.

@syedowaisalishah
Copy link
Contributor Author

syedowaisalishah commented Jun 11, 2025

@ThinkOpenly ,Changes applied as suggested:

return csr_value.RCID & ((1 << RCID_WIDTH) - 1);

also added RCID_WIDTH and MCID_WIDTH as parameters in the extension YAML
All tests are now passing, PR is ready for merge.

@syedowaisalishah
Copy link
Contributor Author

Hi @dhower-qc could you please take a look at this PR and approve it if everything looks good? It has one pending review request blocking the merge, and all checks have passed. Thanks!

Copy link
Collaborator

@dhower-qc dhower-qc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@ThinkOpenly ThinkOpenly added this pull request to the merge queue Jun 18, 2025
Merged via the queue into riscv-software-src:main with commit c06c703 Jun 18, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Ssqosid CSR
4 participants